Add rust runtime #1597

nhynes · 2018-08-14T03:45:20Z

This PR adds a Rust runtime which is useful when creating statically linked TVM modules--either native or Wasm. Indeed, Rust has the best Wasm support. It's also good for SGX.

The current code supports both TVM and NNVM modules. The API around Tensor conversions to DLTensor is a bit awkward since DLTensor isn't owned, but it shouldn't be too hard to iron out.

There's one nit, though: the dshape field in the Tensor struct exists solely because TVM uses u64 as the shape_t even when on 32-bit platforms. I'll make a PR for this eventually...

nhynes · 2018-08-14T03:52:45Z

src/runtime/rust/src/runtime/threading.rs

+  if let Some(q) = SGX_QUEUES.lock().unwrap().pop_front() {
+    ThreadPool::run_worker(q);
+  }
+}


this function interacts with the C++ runtime dylib

nhynes · 2018-08-14T03:56:26Z

src/runtime/rust/src/runtime/array.rs

+  pub(super) strides: Option<Vec<usize>>,
+  pub(super) byte_offset: isize,
+  pub(super) numel: usize,
+  pub(super) dshape: Vec<i64>,


this is the dshape mentioned in the PR description. When the target arch is 32-bit, usize is 32 bits. SInce the TVM module wants a 64-bit shape array, there needs to be an owned copy stored somewhere (i.e. in dshape).

nhynes · 2018-08-14T03:57:15Z

src/runtime/rust/.rustfmt.toml

+report_todo = "Never"
+report_fixme = "Never"
+ignore = []
+verbose_diff = false


this is the default rustfmt.toml produced by rustfmt --dump-default-config

tqchen · 2018-08-14T04:02:35Z

@jroesch @ehsanmok can you help review this?

tqchen · 2018-08-14T04:03:16Z

Let us directly put it under tvm/rust

ehsanmok · 2018-08-14T18:21:42Z

@tqchen absolutely!

First of all, I'd like to thank @nhynes for his initiation. Since three months ago, I've followed his code and learned a lot from his contribution. Well-done Nick!

Here're some questions that I need clarifications:

What's the clear and correct way of compiling tvm-rs? I'm in Rust nightly 1.30 and cannot compile it? do I need to add/install xargo.toml and compile with that?
What about non-cpu Rust runtime context? don't we want gpu support in Rust for example? I was under the impression that we want the API to be as close as other runtime APIs (Java, Js, Python) but this work doesn't do the job! I know that Rustlang non-cpu support itself is not yet as mature as other mentioned languages, but that doesn't justify to give up on what's possible now. What's wrong with having the exact runtime API support as Java (dylib) for example, as well? (this is exactly what I'm trying to complete in tvm-rust).
@tqchen Could you enlighten me on the direction?
There's no documentation and example repo for newcomers. In my opinion, this is separate from tutorial considering Rust audience with no ML related experience.

tqchen · 2018-08-14T19:44:24Z

+1 for docs and comments, I would recommend having one example for users to get started, let us make sure all the public API are documented and commented, that is what we need reviews for :)

I also created an issue to follow the discussion #1601

ehsanmok · 2018-08-14T19:57:59Z

@tqchen yes, it was confusing to me that this PR adds more and is different from what already exists for "runtime frontend" support that I had in mind which I thought is the intention of v0.5 roadmap.

nhynes · 2018-08-14T21:18:06Z

@ehsanmok

What's the clear and correct way of compiling tvm-rs?

cargo build. The error you were getting before was due to the build.rs being ignored by git.

What's wrong with having the exact runtime API support as Java

Nothing at all. That's what we're working on: a unified Rust crate-of-crates which supports both frontend and "backend" Rust. Currently, having a Rust backend is useful for the emerging technologies of Wasm and SGX. These are effectively CPU targets, so that's where the effort has gone.

documentation and example repo

The tests could easily be made into docs. I'll wait for @jroesch's reviews before diving into docs.

rust/.travis.yml

jroesch · 2018-08-15T07:46:37Z

rust/src/runtime/array.rs

+  }
+}
+
+fn tensor_from_array_storage<'a, 's, T, D: ndarray::Dimension>(


is it possible to make these static methods?

rust/src/runtime/array.rs

jroesch · 2018-08-15T07:48:03Z

rust/src/runtime/graph.rs

+  DLDataTypeCode_kDLFloat, DLDataTypeCode_kDLInt, DLDataTypeCode_kDLUInt, DLTensor,
+};
+
+const NDARRAY_MAGIC: u64 = 0xDD5E40F096B4A13F; // Magic number for NDArray file


These need a deeper explanation.

jroesch · 2018-08-15T08:01:59Z

Just did a quick pass before bed, code looks good, but could use lots of docs explaining it. I think it would be very hard for someone who didn't know the existing runtime to understand. I'll take another stab tomorrow or after you do docs? just let me know what would be more useful.

nhynes · 2018-08-15T16:36:30Z

thanks for your review @jroesch! In the interest of making efficient use of time, I'll make a docs pass before you do further review.

ehsanmok · 2018-08-15T17:30:04Z

src/runtime/rust/Cargo.toml

+authors = ["Nick Hynes <nhynes@berkeley.edu>"]
+
+[features]
+par-launch-alloc = []


Has #1226 been resolved already? if not, then I don't think it's a good idea to put it as a feature and maybe in a separate branch until it's resolved!

ehsanmok · 2018-08-15T17:39:14Z

src/runtime/rust/src/runtime/allocator.rs

+const DEFAULT_ALIGN_BYTES: usize = 4;
+
+#[derive(PartialEq, Eq)]
+pub struct Allocation {


Would it better to impl Alloc for it? or at least add the stabilized #[global_allocator] attribute?

This isn't actually an allocator--it's just a way to get bytes out of the global allocator in a less-unstable way.

ehsanmok · 2018-08-15T17:42:34Z

src/runtime/rust/src/runtime/array.rs

+  pub(super) shape: Vec<usize>,
+  pub(super) strides: Option<Vec<usize>>,
+  pub(super) byte_offset: isize,
+  pub(super) numel: usize,


It'd be better to rename numel to size which is now the standard definition in ndarray notion of both numpy and bluss's Rust ndarray.

ehsanmok · 2018-08-15T17:47:42Z

src/runtime/rust/src/runtime/array.rs

+  }
+}
+
+impl<'a, 't> TryFrom<&'a Tensor<'t>> for ndarray::ArrayD<f32> {


Just wonder about other numeric types, f64 or ints?

ehsanmok · 2018-08-15T17:51:57Z

src/runtime/rust/src/runtime/module.rs

+  fn get_function<S: AsRef<str>>(&self, name: S) -> Option<PackedFunc>;
+}
+
+pub struct SystemLibModule {}


Empty struct with no {}, maybe?

ehsanmok · 2018-08-15T17:52:46Z

src/runtime/rust/src/runtime/packed_func.rs

+pub type PackedFunc = Box<Fn(&[TVMArgValue]) -> TVMRetValue>;
+
+#[macro_export]
+macro_rules! call_packed {


Nice and clever :)

ehsanmok · 2018-08-15T17:54:31Z

src/runtime/rust/src/runtime/packed_func.rs

+impl_prim_ret_value!(u32, 1);
+impl_prim_ret_value!(f32, 2);
+impl_boxed_ret_value!(String, 11);
+


Other primitives, perhaps?

ehsanmok · 2018-08-15T18:00:49Z

src/runtime/rust/src/runtime/array.rs

+}
+
+impl<'a> Tensor<'a> {
+  pub fn shape(&self) -> Vec<usize> {


Inlinable with #[inline] and the like methods.

I generally don't use #[inline] because 1) I'm not smarter than the compiler and 2) the end user can always use link-time optimization (ref).

Also keep in mind that fast binary is not always as important as size: in Wasm, for instance, small binaries download faster. Similarly in SGX, smaller binaries affords more enclave memory for application use. In any case, LTO is unconditionally more useful than inlining.

Partially agree! while it's debatable, my reference is bluss's ndarray and the like libs he's wrote with careful inlining. I'm not familiar with wasm though.

I'm not sure that it's a debate: LTO is unconditionally better. The ndarray crate is relatively old--perhaps bluss was using best practices from a time when the Rust linker was less sophisticated.

References:

https://stackoverflow.com/questions/37639276/when-should-inline-be-used-in-rust

https://stackoverflow.com/questions/1759300/when-should-i-write-the-keyword-inline-for-a-function-method

https://stackoverflow.com/questions/7046547/link-time-optimization-and-inline

https://stackoverflow.com/questions/5948703/difference-in-inlining-functions-by-compiler-or-linker

Thanks for the links! I'll look into them. The major source of (anything in fact and) inlining is in std, for example. I think, if in doubt in general, then skip it for now.

ehsanmok · 2018-08-15T18:02:33Z

src/runtime/rust/src/runtime/array.rs

+}
+
+impl DataType {
+  fn itemsize(&self) -> usize {


The same #[inline(always)]. (Not repeating for the like)

nhynes · 2018-08-20T00:03:51Z

@jroesch I added some docs. Hopefully they're sufficient for your critical review.

@ehsanmok Thanks for the feedback. I addressed most of it. I'll attend to the other comments when we're closer to merging.

nhynes · 2018-08-22T16:38:51Z

Rendered docs

ehsanmok · 2018-08-22T18:10:34Z

@nhynes thanks! a couple of points/questions:

Besides adding a separate example repo with a couple of end-to-end use cases (cleaner version of integrations tests maybe?), I suggest adding more docs specially in src/lib.rs describing what tvm is, links to tvm.ai, more tutorials etc. and what this runtime lib provides.
For cleaner rendering of docs, I suggest to not include the dependencies and/or use #[doc(hidden)] to control the visibilities of some parts of this lib. For example, consider hiding the constants in tvm::ffi::runtime.
Question: (to @tqchen as well) are we considering to support rust as a backend replacement of c++ ever? if so, I hardly think it'd be a good plausible idea and if not, then shall we get rid of the separate tvm::runtime module namespace in this PR and simply put everything there in the root lib?

tqchen · 2018-08-22T18:19:46Z

I think rust version of runtime has its own merit, mainly for support wasm support(seems rust support is better than c++), so it is good to enable this option for users who need it

ehsanmok · 2018-08-22T19:22:54Z

@tqchen let me rephrase my question. Wouldn't it be clear from the context of the library that rust will provide runtime support? in relation to #1601, I'm thinking of three types of API supports: 1) compiler backend: c++ (and python for prototyping) 2) runtime frontend: python, java, js, etc. 3) runtime backend (this PR) and frontend (my work): rust. So rust is not going to be in 1), right?
Then by default, tvm/rust is about runtime whether backend (wasm etc.) or frontend. That's why I'm asking to simplify the module namespace and place everything in tvm::runtime as simply in root tvm.

nhynes · 2018-08-22T19:53:21Z

place everything in tvm::runtime as simply in root tvm

What you're working on isn't a runtime, though. It'd still go in the tvm/rust directory but have a different crate. We could have rust/tvm/runtime be tvm-runtime and your frontend, simply tvm.

rust compiler backend

Not until rust has good python FFI support. That's the main benefit of C[++]

tqchen · 2018-09-25T03:16:45Z

@ehsanmok @jroesch @nhynes it is a good time to revive this thread, let us add follow up comments and work to merge this in

ehsanmok · 2018-09-25T17:45:32Z

@tqchen the last number of changes addressed some of my concerns and comments above. Besides improving docs and some other smaller things (like #[inline] attributes) that can be done later, my main concern is related to #1226 and seems allowing it in the build. If @nhynes thinks it was already resolved, then I think there shouldn't be a major blocking issue to merge. If not, then at least we can remove the feature and fix it later!

nhynes · 2018-09-25T17:47:10Z

my main concern is related to #1226 and seems allowing it in the build

I haven't looped around to fixing that, but since the rust runtime is mostly useful for Wasm (single threaded) and SGX (which doesn't manage its own threads), it's not a blocker.

ehsanmok · 2018-09-25T19:00:56Z

@nhynes Ok! So we shouldn't allow the related par-launch-alloc feature in master until it is resolved.

nhynes · 2018-09-26T05:16:25Z

Actually, I just fixed the bug. Turns out the issue was that the temporal workspaces only work correctly if they have the same alignment as the dtype they're supposed to hold. Go figure.

tqchen · 2018-10-02T03:42:22Z

Let us push to get this in, @nhynes please send a separate PR to upstream to update the Dockerfile.demo_cpu with necessary rust env.

@nhynes I take a quick look, although I am not rust expert, it seems to me that the documentation are still sparse. Please at least document

The tvm runtime module (e.g. This is a rust implementation of tvm runtime, can be used for SGX, WASM etc...)
The user-facing API functions (borrow docs from TVM API)
Some of the subtle implementation points that @jroesch mentioned.

After this is done and test get passed we can merge it in and followup with @ehsanmok on rust frontend

nhynes · 2018-10-02T04:18:51Z

documentation are still sparse

The convention is to use the auto-generated docs with manual docs only when necessary. Here's a link to the current docs. It's also populated with examples.

I'll add a top-level module doc, add a dockerfile config, and address those code review comments.

nhynes · 2018-10-02T16:06:49Z

Dockerfile.demo_cpu

should I also update CI? Rust builds might be a bit flaky since this crate needs to use nightly, though

tqchen · 2018-10-04T18:21:07Z

Oh, yes, I meant to say ci_cpu. Using nightly is fine. However, note that we build the docker image infrequently, which means we will be stuck to a certain version of nightly. Using a stable version or hashtag is preferred

tqchen · 2018-10-05T21:33:13Z

Please add rust testcases to Jenkinsfile to here https://github.com/dmlc/tvm/blob/master/Jenkinsfile#L141

nhynes · 2018-10-06T01:44:19Z

the testcases are added in nhynes#1
I'll make a PR for that here when this branch and #1825 are merged

tqchen · 2018-10-06T17:30:44Z

Thanks @jroesch @nhynes @ehsanmok , this is now merged

nhynes mentioned this pull request Aug 14, 2018

TVM v0.5 Roadmap #1596

Closed

32 tasks

nhynes commented Aug 14, 2018

View reviewed changes

tqchen added the status: need review label Aug 14, 2018

tqchen mentioned this pull request Aug 14, 2018

[RFC] Rust Support #1601

Closed

jroesch reviewed Aug 15, 2018

View reviewed changes

rust/.travis.yml Show resolved Hide resolved

jroesch reviewed Aug 15, 2018

View reviewed changes

ehsanmok reviewed Aug 15, 2018

View reviewed changes

nhynes force-pushed the rust-runtime branch from 616f1f4 to 05151ef Compare August 20, 2018 00:02

nhynes force-pushed the rust-runtime branch from 05151ef to 2c7117c Compare August 20, 2018 00:07

tqchen self-assigned this Aug 21, 2018

nhynes mentioned this pull request Sep 24, 2018

Update SGX cmake #1763

Merged

ehsanmok approved these changes Sep 26, 2018

View reviewed changes

tqchen added status: need update need update based on feedbacks and removed status: need review labels Oct 2, 2018

nhynes force-pushed the rust-runtime branch from 7118bdf to a1e4d02 Compare October 5, 2018 00:06

nhynes mentioned this pull request Oct 5, 2018

Update SGX example #1825

Merged

nhynes and others added 10 commits October 5, 2018 18:41

Add rust runtime

90d1f2c

Move files

81bcb88

Re-add build script

f57d622

Add now un-ignored files to tests

f018244

Add docs

72aa38b

Rename numel to size

0b515c9

Address code review comments

ce067be

Allow building SGX modules

706c8de

Updates

a0d3d41

Updates

88aff7c

nhynes force-pushed the rust-runtime branch from 54642fd to 88aff7c Compare October 6, 2018 01:41

tqchen merged commit 5563b72 into apache:master Oct 6, 2018

tqchen added status: accepted and removed status: need update need update based on feedbacks labels Oct 6, 2018

nhynes mentioned this pull request Oct 6, 2018

[SGX] Add ignored files to sgx example #1852

Merged

nhynes deleted the rust-runtime branch October 12, 2018 05:33

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018

Add rust runtime (apache#1597)

2c2ca3f

ZihengJiang mentioned this pull request Feb 2, 2019

TVM 0.5 Release Note #2448

Closed

Add rust runtime #1597

Add rust runtime #1597

Conversation

nhynes commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Aug 14, 2018

tqchen commented Aug 14, 2018

ehsanmok commented Aug 14, 2018 • edited Loading

tqchen commented Aug 14, 2018 • edited Loading

ehsanmok commented Aug 14, 2018 • edited Loading

nhynes commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch commented Aug 15, 2018

nhynes commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhynes Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehsanmok Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

nhynes commented Aug 20, 2018

nhynes commented Aug 22, 2018

ehsanmok commented Aug 22, 2018 • edited Loading

tqchen commented Aug 22, 2018

ehsanmok commented Aug 22, 2018

nhynes commented Aug 22, 2018

tqchen commented Sep 25, 2018

ehsanmok commented Sep 25, 2018

nhynes commented Sep 25, 2018

ehsanmok commented Sep 25, 2018

nhynes commented Sep 26, 2018

tqchen commented Oct 2, 2018

nhynes commented Oct 2, 2018

nhynes commented Oct 2, 2018

tqchen commented Oct 4, 2018 • edited Loading

tqchen commented Oct 5, 2018

nhynes commented Oct 6, 2018 • edited Loading

tqchen commented Oct 6, 2018

nhynes commented Aug 14, 2018 •

edited

Loading

ehsanmok commented Aug 14, 2018 •

edited

Loading

tqchen commented Aug 14, 2018 •

edited

Loading

ehsanmok commented Aug 14, 2018 •

edited

Loading

nhynes Aug 15, 2018 •

edited

Loading

ehsanmok Aug 15, 2018 •

edited

Loading

ehsanmok commented Aug 22, 2018 •

edited

Loading

tqchen commented Oct 4, 2018 •

edited

Loading

nhynes commented Oct 6, 2018 •

edited

Loading